Repository navigation
Conversation
The name from a bun.lock packages entry was copied into fixed-size path buffers by both linkers without a length check. The hoisted linker panicked in ByteCursor::put when it printed the cache folder name. The isolated linker panicked on a worker thread in Path::append_fmt / Path::append when it built the store path of a dependency, before the main thread had validated that entry's name. is_safe_install_folder_name and alias_is_safe_install_target now reject a name longer than 255 bytes (NAME_MAX, above npm's 214 byte limit), so the bun.lock parser reports "Invalid package name" and the tree builder reports "Invalid dependency name". The hoisted installer also checks an npm package's own name before it prints the cache folder name, for names that arrive through other lockfile formats. The isolated installer validates every name and alias before it starts the first task, so that check no longer races the workers.
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 11:38 AM PT - Sep 6th, 2026
⏳ @robobun, your commit 904da3e is still building in |
|
Status Reproduced on canary (1.4.3-canary.1) with a
The new tests in CI (build 111651): green for this diff on every lane that ran, including macOS aarch64. The one red test, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes untrusted-input path validation in both linkers and reorders the isolated installer's validation relative to worker-thread task dispatch, a human look at the security-sensitive bits would still be worthwhile.
What was reviewed:
- The 255-byte bound in
is_safe_install_folder_name/alias_is_safe_install_targetand its callers — every existing caller (bun.lock parser, Tree builder, tarball extract, bin) now picks up the length check. - The hoisted-linker addition that also validates
pkg_name(not just the alias) for npm resolutions before it reaches the cache-folder path. - The isolated-install hoist: the new upfront loop indexes
entry_node_idsby the same entry index the task-start loop uses, so it validates the same set of names/aliases the old in-loop check did, just before anystart_taskruns. - Tests:
describe.eachover both linkers, boundary at 214 accepted / 256+ rejected, uses local Verdaccio, asserts stderr before exit code.
Extended reasoning...
Overview
This PR hardens bun install against overlong package names in bun.lock that previously reached fixed-size MAX_PATH_BYTES path buffers unchecked and panicked (index-out-of-bounds in Path::append / ByteCursor::put). It adds MAX_INSTALL_FOLDER_NAME_LEN = 255 in src/install/dependency.rs and applies it in both is_safe_install_folder_name and alias_is_safe_install_target (the latter previously bounded at MAX_PATH_BYTES, which was insufficient because store paths embed the name twice plus a prefix). In src/install/PackageInstaller.rs the hoisted linker now also validates an npm resolution's own package name — not just the dependency alias — since that name becomes the cache folder path. In src/install/isolated_install.rs the per-entry name/alias validation is lifted out of the task-start loop into a separate pass over all entries so no worker thread can build a store path for an unvalidated dependency name. Eight new test cases in test/cli/install/bun-install-registry.test.ts cover both linkers at 214 (accepted) and 256/3000/1,000,000 bytes (rejected with "Invalid package name").
Security risks
The change is squarely in the "untrusted lockfile string becomes a filesystem path" category REVIEW.md calls out. It strictly tightens validation (adds a length upper bound; no check is loosened), so the risk surface is reduced rather than expanded. The new bound of 255 is above npm's 214-byte package-name limit and equals NAME_MAX, so no legitimate package name is newly rejected. The isolated-linker reordering closes a race where a worker could build a store path for a dependency whose entry the main thread hadn't validated yet. I don't see a new bypass: the length check is applied at the shared predicate every consumer already routes through (bun.lock parser, tree builder, tarball extract, bin linker, both installers), so the whole class picks it up. The remaining gap the PR notes (registry-served manifests reaching cached_npm_package_folder_name on fresh resolve) is explicitly deferred to #38615, which reuses the same predicate.
Level of scrutiny
Medium-high. The diff is small (~120 lines) and mechanical in shape, but it sits on a security boundary (path construction from attacker-controllable lockfile content) and adjusts the ordering of validation relative to multi-threaded task dispatch in the isolated installer. The race-fix reasoning depends on entry_dependencies, entry_node_ids, and store.entries all being indexed by the same entry index — which the surrounding code confirms (the old in-loop check used entry_dependencies[entry_id.get() as usize] and the task loop uses entry_node_ids[entry_id.get() as usize]), but a maintainer familiar with the isolated store layout should confirm no entry is reachable by a worker that isn't covered by the upfront enumeration.
Other factors
Tests follow repo conventions well: describe.each over both linkers, Buffer.alloc(n, fill).toString() for long strings, local Verdaccio via registryUrl(), pipes drained concurrently, stderr asserted before exit code, and boundary coverage (npm's 214-byte limit accepted, one past the new 255-byte bound rejected). No CODEOWNERS cover the touched paths. The PR description is unusually thorough (repro, root cause, why 255 not MAX_PATH_BYTES, what's deferred). Given the security-adjacent nature and the thread-ordering argument, deferring for a human confirmation rather than auto-approving.
|
I built this branch merged with #38615 on main at 630e921 to check the path listed under "Not addressed here". Result. The two changes together close that path. Neither closes it alone. The case: the registry manifest of a package lists a dependency whose name is longer than the path buffer, and the root project depends on that package. On main, With both merged, the enqueue check from #38615 calls I ran names of 4090 and 4200 bytes on the hoisted and isolated linkers. The name was a direct dependency, a transitive dependency, or reached through Merge conflicts. The two PRs conflict in
Test gap. No test in either PR covers the registry case, because it fails on each branch alone. The PR that lands second should add one: a stub registry whose manifest for package A lists a dependency with a 4200 byte name, a root project that depends on A, and assertions on One observation that this PR does not change: names of 240 and 255 bytes pass the check but fail later with |
|
Closing in favor of #43067, which contains this change and its tests. The 255-byte name bound is what closes the registry manifest and lockfile triggers there. |
Problem
bun.lockpackagesentry with a very long ident name abortsbun install --frozen-lockfile. Isolated linker:panic: internal error: entered unreachable codeinbun_paths::Path::append_fmt(src/paths/Path.rs:1014) on a thread-pool thread, underInstaller::append_store_path. Hoisted linker:panic: range end index 1000004 out of range for slice of length 4096inByteCursor::put(PackageManagerDirectories.rs:459).is_safe_install_folder_name(src/install/dependency.rs:537), which thebun.lockparser applies to the name, has no length bound. Both linkers copy the name into fixed-size path buffers without a check. The isolated linker's bounded check runs inside the loop that starts tasks, so a worker races it.Fix
is_safe_install_folder_nameandalias_is_safe_install_targetreject names longer than 255 bytes (NAME_MAX, above npm's 214 byte limit). The parser reportsInvalid package name.test/cli/install/bun-install-registry.test.ts, 8 cases on both linkers, 6 fail on canary. Six other install suites pass. Self-reviewed: 9 concerns, 7 addressed, 2 left out (see Notes).Background
bun.lockpackagesentry is"<key>": ["<name>@<resolution>", ...]. The name becomes thenode_modules/<name>folder and the cache folder name.MAX_PATH_BYTESis the size of bun's pooled path buffers: 4096 on Linux, 1024 on macOS.bun_paths::PathinASSUMEmode indexes straight into one.Notes
package.json{"dependencies":{"no-deps":"1.0.0"}},bun.lockwith"packages": {"no-deps": ["e" + "A"*N + "vil@1.0.0", "<tarball url>", {}, "<sha512>"]}, thenbun install --frozen-lockfile --linker isolated|hoisted. On canary: N around 2500 to 4095 panics the isolated linker inPath::append(index out of bounds: the len is 4096 but the index is 4096), N of 4096 or more panics it inPath::append_fmtsome of the time and printsis not a valid install folder namethe rest of the time (the race), and N of 4096 or more panics the hoisted linker inByteCursor::putevery time. Found by a text-lockfile fuzz.Invalid dependency namefor an alias over the bound, through the same predicate.MAX_PATH_BYTES. Review showed that names between about 2000 and 4095 bytes still aborted the isolated linker (the store path embeds the name twice plus the project directory) and names just under 4096 still overflowed the hoisted cache folder name, so the bound is now 255.cached_npm_package_folder_namethroughdetermine_preinstall_stateon a fresh resolve. install: reject dependency names containing control characters #38615 adds name validation at enqueue through the same predicate, so it picks up this bound. The v1 git.bun-tag/ committish inputs of the sameByteCursorare a separate input and are not touched.isolated-install,bun-install-registry,bun-install,bun-lock,bun-workspaces,bun-add,migration/migrate. Thebun-install.test.tsgitlab/bitbucket/some.urlcases fail in this container with and without the change (no public internet).bun-link.test.tsshould link dependency without crashingfails on a debug build of main too.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts